Skip to content
This repository was archived by the owner on Sep 21, 2024. It is now read-only.

chore: Genercize storage across gateway and rename "default" storages#622

Closed
jsantell wants to merge 1 commit intomainfrom
generic-storage
Closed

chore: Genercize storage across gateway and rename "default" storages#622
jsantell wants to merge 1 commit intomainfrom
generic-storage

Conversation

@jsantell
Copy link
Copy Markdown
Contributor

@jsantell jsantell commented Sep 5, 2023

chore: Rename NativeStorage -> SledStorage, WebStorage -> IndexedDbStorage. Make storage generic over gateway. In preparation for exploring pluggable backends in #607

pub async fn to_storage(&self) -> Result<NativeStorage> {
NativeStorage::new(NativeStorageInit::Path(PathBuf::from(self)))
pub async fn to_storage(&self) -> Result<SledStorage> {
SledStorage::new(SledStorageInit::Path(PathBuf::from(self)))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and make_disposable_store) are the last non-generic storage invocations AFAICT. As PlatformStorage could be a non-primitive (like IpfsStorage wrapper), some options:

  • Storage adds a new_from_path method that works on all Storage impls (or new_from_str)
  • noosphere::platform exposes something like make_platform_storage(&str), though we want to distinguish PlatformStorage (which may be wrapped by a non-primitive store) from the underlying storage primitive, though currently this would involve a lot of duplicate defs if we want to swap around backends during testing, currently apple/aarch64 and non-native-apple multiplied by storage backends

Comment thread rust/noosphere/src/platform.rs
@justinabrahms
Copy link
Copy Markdown
Contributor

justinabrahms commented Sep 5, 2023 via email

Comment on lines +61 to +62
St: Send + Sync,
S: Storage + 'static,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that there is no t (you named it St) in Send + Sync and there are three in Storage + 'static, which will probably confuse people w/ naming.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! The St type reflects the underlying axum State as rationale for naming, but you make a great point, will update to something less ambiguous

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally wouldn't balk at a non-abbreviated identifier for the generic type.

Copy link
Copy Markdown
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this is great. I'm going to ask that we don't land before #624 but I will help with the rebase.

@jsantell
Copy link
Copy Markdown
Contributor Author

LGTM, this is great. I'm going to ask that we don't land before #624 but I will help with the rebase.

It'll be easier rebasing #623 (which was built off of/evolved from this PR) and closing this PR most likely

@jsantell
Copy link
Copy Markdown
Contributor Author

Closing in lieu of superceding work in #623

@jsantell jsantell closed this Sep 11, 2023
@jsantell jsantell deleted the generic-storage branch September 11, 2023 23:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants